Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add container app module #1792

Merged
merged 7 commits into from
Nov 24, 2023
Merged

Conversation

trapeznikov
Copy link
Contributor

1301

PR Checklist


Description

Does this introduce a breaking change

  • YES
  • NO

Testing

Copy link
Member

@arnaudlh arnaudlh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @trapeznikov, Im running examples and getting:

│ Error: Reference to undeclared input variable
│ 
│   on module.tf line 106, in module "example":
│  106:     container_app                         = var.container_app
│ 
│ An input variable with the name "container_app" has not been declared. This variable can be declared with a variable
│ "container_app" {} block.
╵
╷
│ Error: Reference to undeclared input variable
│ 
│   on module.tf line 107, in module "example":
│  107:     container_app_dapr_component          = var.container_app_dapr_component
│ 
│ An input variable with the name "container_app_dapr_component" has not been declared. This variable can be declared with
│ a variable "container_app_dapr_component" {} block.
╵
╷
│ Error: Reference to undeclared input variable
│ 
│   on module.tf line 108, in module "example":
│  108:     container_app_environment             = var.container_app_environment
│ 
│ An input variable with the name "container_app_environment" has not been declared. This variable can be declared with a
│ variable "container_app_environment" {} block.
╵
╷
│ Error: Reference to undeclared input variable
│ 
│   on module.tf line 109, in module "example":
│  109:     container_app_environment_certificate = var.container_app_environment_certificate
│ 
│ An input variable with the name "container_app_environment_certificate" has not been declared. This variable can be
│ declared with a variable "container_app_environment_certificate" {} block.
╵
╷
│ Error: Reference to undeclared input variable
│ 
│   on module.tf line 110, in module "example":
│  110:     container_app_environment_storage     = var.container_app_environment_storage
│ 
│ An input variable with the name "container_app_environment_storage" has not been declared. This variable can be declared
│ with a variable "container_app_environment_storage" {} block.
╵
Terraform plan return code: 1
Error 1 on or near line 57: Error running terraform plan; exiting with status 1

@trapeznikov
Copy link
Contributor Author

@arnaudlh need to add container_app to https://github.com/Azure/caf-terraform-landingzones/tree/main/caf_solution. What's the process of adding a new feature to these 2 repos? Should the change first go to caf-terraform-landingzones?

@arnaudlh
Copy link
Member

arnaudlh commented Sep 8, 2023

@arnaudlh need to add container_app to https://github.com/Azure/caf-terraform-landingzones/tree/main/caf_solution. What's the process of adding a new feature to these 2 repos? Should the change first go to caf-terraform-landingzones?

  1. update the module with examples working, so people can leverage it as standalone
  2. update the azure repo with the new var names and module version constraint.

@trapeznikov
Copy link
Contributor Author

trapeznikov commented Sep 9, 2023

@arnaudlh I fixed the variables file in the example module, it should run fine now. Sorry for the confusion, I was just testing it differently.

@arnaudlh arnaudlh changed the title add container app module Add container app module Oct 11, 2023
@mresetar-croz
Copy link

I am currently working with this PR code. I hope it gets merged soon. Maybe not the right place but please follow hashicorp/terraform-provider-azurerm#21747 as workload profiles support is something in the future that should be supported by CAF as well.

locals.tf Outdated Show resolved Hide resolved
@trapeznikov trapeznikov force-pushed the feat/azure_container_app branch from d9ec19b to bdbe498 Compare November 1, 2023 19:30
@trapeznikov
Copy link
Contributor Author

@arnaudlh I resolved all the comment and rebased the branch. Can you please review?

@trapeznikov
Copy link
Contributor Author

@arnaudlh @LaurentLesle can you please review this PR?

log_analytics_workspace_id = can(var.settings.log_analytics_workspace_id) ? var.settings.log_analytics_workspace_id : var.diagnostics.log_analytics[var.settings.log_analytics_key].id
infrastructure_subnet_id = try(var.subnet_id, null)
internal_load_balancer_enabled = try(var.settings.internal_load_balancer_enabled, false)
tags = merge(local.tags, try(var.settings.tags, null))
Copy link
Contributor

@iriahk89 iriahk89 Nov 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please help to add support for another 2 parameter: dapr_application_insights_connection_string & zone_redundancy_enabled. And I received error for internal_load_balancer_enabled for example 101 aca. but after I change false to null it works. I think for optional parameter we can just pass null if not specify like this:
internal_load_balancer_enabled = try(var.settings.internal_load_balancer_enabled, null)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trapeznikov good PR - thank you! if you dont mind adding the 2 attributes mentionned by @iriahk89, I think we are good to go!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arnaudlh @iriahk89 done. Please review.

Copy link
Contributor

@iriahk89 iriahk89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trapeznikov I reviewed the PR and would really appreciate if you could add those 2 features and resolve the minor bug as one of my customers also waiting for this resource. Let me know if you need helps. Thanks!

@trapeznikov trapeznikov requested a review from iriahk89 November 23, 2023 19:59
Copy link
Contributor

@iriahk89 iriahk89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. we can add the integration with app insight combined object later for dapr_application_insights_connection_string if needed

@arnaudlh arnaudlh added this to the 5.7.7 milestone Nov 24, 2023
Copy link
Member

@arnaudlh arnaudlh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@arnaudlh arnaudlh linked an issue Nov 24, 2023 that may be closed by this pull request
1 task
@arnaudlh arnaudlh merged commit ea8a5dd into aztfmod:main Nov 24, 2023
@arnaudlh
Copy link
Member

thanks for your contribution @trapeznikov and for your reviews @iriahk89 @jvdhaterd

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Feature request-Add support for Azure Containers Apps
6 participants